Allow rebase of branches during migration #7447
Conversation
github-actions
bot
added
the
group/backend
WalkthroughAdds MigrationRequiringRebase, MigrationRunner, and MigrationFailureError. Branch model gains nullable graph_version and BranchStatus adds NEED_UPGRADE_REBASE. CLI migration flow reworked: detect_migration_to_run added, migrate_database now accepts a migrations sequence, and trigger_rebase_branches introduced; upgrade CLI gains --rebase-branches. Adds branch-focused tasks (migrate_branch, rebase_branch signature updated with send_events), emits BranchMigratedEvent and BRANCH_MIGRATE workflow, exposes graph_version in GraphQL/schema, and adds unit tests and documentation updates for these flows. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CodSpeed Performance ReportMerging #7447 will not alter performanceComparing Summary
|
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
5 times, most recently
from
c975dfe to
2d5ec00
Compare
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
from
792c84c to
1597160
Compare
github-actions
bot
added
the
type/documentation
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
3 times, most recently
from
f66d756 to
1cee062
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
backend/infrahub/task_manager/models.py (1)
137-156: Consider refactoring to eliminate code duplication.The three blocks handling
branch_merged,branch_migrated, andbranch_rebasedfollow an identical pattern with only the event type string differing. This violates the DRY principle and makes maintenance harder.Consider extracting this logic into a helper method:
def _add_branch_event_filter( self, event_type: list[str], event_type_key: str, event_name: str, filter_data: dict[str, Any] ) -> None: """Add branch event filter for merge, migrate, or rebase events.""" branches: list[str] = filter_data.get("branches") or [] if "infrahub.branch.created" not in event_type: event_type.append(event_name) if branches: self.resource = EventResourceFilter( labels=ResourceSpecification({"infrahub.branch.name": branches}) )Then use it like:
for event_key, event_name in [ ("branch_merged", "infrahub.branch.merged"), ("branch_migrated", "infrahub.branch.migrated"), ("branch_rebased", "infrahub.branch.rebased"), ]: if filter_data := event_type_filter.get(event_key): self._add_branch_event_filter(event_type, event_key, event_name, filter_data)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/infrahub/cli/db.py(7 hunks)backend/infrahub/cli/upgrade.py(6 hunks)backend/infrahub/core/branch/enums.py(1 hunks)backend/infrahub/core/branch/models.py(3 hunks)backend/infrahub/core/branch/tasks.py(5 hunks)backend/infrahub/core/constants/__init__.py(1 hunks)backend/infrahub/core/migrations/exceptions.py(1 hunks)backend/infrahub/core/migrations/graph/__init__.py(3 hunks)backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py(3 hunks)backend/infrahub/core/migrations/runner.py(1 hunks)backend/infrahub/core/migrations/shared.py(2 hunks)backend/infrahub/events/branch_action.py(2 hunks)backend/infrahub/graphql/types/branch.py(2 hunks)backend/infrahub/task_manager/event.py(2 hunks)backend/infrahub/task_manager/models.py(1 hunks)backend/infrahub/workflows/catalogue.py(2 hunks)backend/tests/unit/core/migrations/test_runner.py(1 hunks)backend/tests/unit/workflows/test_models.py(1 hunks)docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx(1 hunks)docs/docs/reference/infrahub-events.mdx(2 hunks)schema/schema.graphql(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/graphql/types/branch.pybackend/infrahub/task_manager/event.pybackend/infrahub/task_manager/models.pybackend/infrahub/core/migrations/runner.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/branch/models.pybackend/infrahub/core/constants/__init__.pybackend/infrahub/core/migrations/exceptions.pybackend/infrahub/events/branch_action.pybackend/tests/unit/workflows/test_models.pybackend/infrahub/cli/upgrade.pybackend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.pybackend/infrahub/core/migrations/graph/__init__.pybackend/tests/unit/core/migrations/test_runner.pybackend/infrahub/cli/db.pybackend/infrahub/core/branch/enums.pybackend/infrahub/core/migrations/shared.pybackend/infrahub/core/branch/tasks.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/graphql/types/branch.pybackend/infrahub/task_manager/event.pybackend/infrahub/task_manager/models.pybackend/infrahub/core/migrations/runner.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/branch/models.pybackend/infrahub/core/constants/__init__.pybackend/infrahub/core/migrations/exceptions.pybackend/infrahub/events/branch_action.pybackend/tests/unit/workflows/test_models.pybackend/infrahub/cli/upgrade.pybackend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.pybackend/infrahub/core/migrations/graph/__init__.pybackend/tests/unit/core/migrations/test_runner.pybackend/infrahub/cli/db.pybackend/infrahub/core/branch/enums.pybackend/infrahub/core/migrations/shared.pybackend/infrahub/core/branch/tasks.py
docs/docs/reference/**/*.{md,mdx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write reference documentation in
docs/docs/reference/
Files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
docs/**/*.mdx
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Docusaurus markdown/MDX features in documentation
docs/**/*.mdx: How-to guides should begin title with "How to..." and follow the Diataxis How-to structure (intro, prerequisites, step-by-step, validation, advanced, related)
Use imperative, task-focused instructions in guides; avoid digressions
Topics should use titles like "About..." or "Understanding..." and follow the Diataxis Topics structure (concepts, background, architecture, mental models, connections, alternatives, further reading)
Define new terms on first use; prefer domain-relevant terminology; keep naming consistent with Infrahub UI/data model
Files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
docs/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Develop documentation in
docs/, preview withinvoke docs.build docs.serve, and validate withinvoke docs.validate
Files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
**/*.mdx
📄 CodeRabbit inference engine (.github/instructions/documentation.instructions.md)
**/*.mdx: Structure documentation primarily as How-to Guides and Topics (explanations) following the Diataxis framework
Use a professional, approachable tone; avoid jargon unless defined; use plain language with technical precision
Write concise, direct, active sentences
Be informative over promotional; focus on explaining how and why
Maintain consistent, predictable structure across sections and documents
For Guides: use conditional imperatives (e.g., "If you want X, do Y")
For Guides: focus on practical tasks and problems, not tools
For Guides: address the user directly with imperative verbs (e.g., "Configure...", "Create...")
For Guides: keep focus on the specific goal; avoid digressions into explanations
For Guides: title in YAML frontmatter should clearly state the problem and begin with "How to..."
For Guides: include an Introduction stating the problem/goal, context, and what the user will achieve
For Guides: include a Prerequisites/Assumptions section listing requirements and prior knowledge
For Guides: provide step-by-step instructions with numbered steps; include code snippets/screenshots/tabs/callouts as needed
For Guides: include a Validation/Verification section with checks, example outputs, and potential failure points
For Guides: include a Related Resources section linking to relevant guides/topics/reference
For Topics: title in YAML frontmatter should clearly indicate the topic; consider starting with "About..." or "Understanding..."
For Topics: include an Introduction with overview, why it matters, and questions answered
For Topics: include sections for Concepts & Definitions
For Topics: include Background & Context (history, design rationale, constraints)
For Topics: include Architecture & Design details/diagrams when applicable
For Topics: include Mental Models (analogies/comparisons)
For Topics: connect to other concepts and integration points
For Topics: include Alternative Approaches with pros/cons where relevant
For Topics: include a Further...
Files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
docs/!(node_modules)/**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
docs/!(node_modules)/**/*.{md,mdx}: Lint documentation prose with Vale
Ensure Vale style checks pass for documentation
Files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/workflows/test_models.pybackend/tests/unit/core/migrations/test_runner.py
🧠 Learnings (2)
📚 Learning: 2025-08-21T11:04:26.357Z
Learnt from: CR
PR: opsmill/infrahub#0
File: .github/instructions/documentation.instructions.md:0-0
Timestamp: 2025-08-21T11:04:26.357Z
Learning: Applies to **/*.mdx : Ensure content is accurate and reflects the latest Infrahub version
Applied to files:
docs/docs/reference/infrahub-cli/infrahub-upgrade.mdxdocs/docs/reference/infrahub-events.mdx
📚 Learning: 2025-07-22T08:13:56.198Z
Learnt from: CR
PR: opsmill/infrahub#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-22T08:13:56.198Z
Learning: Documentation generated for Infrahub must reflect its novel approach, providing clarity around new concepts and demonstrating integration with familiar patterns from existing tools like Git, infrastructure-as-code, and CI/CD pipelines
Applied to files:
docs/docs/reference/infrahub-events.mdx
🧬 Code graph analysis (12)
backend/infrahub/task_manager/models.py (1)
backend/infrahub/core/diff/model/path.py (1)
branches(718-722)
backend/infrahub/core/migrations/runner.py (3)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/shared.py (11)
MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)execute_against_branch(239-241)success(27-31)validate_migration(151-152)validate_migration(193-194)validate_migration(220-221)validate_migration(236-237)backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py (2)
execute_against_branch(450-451)validate_migration(373-374)
backend/infrahub/workflows/catalogue.py (1)
backend/infrahub/workflows/models.py (1)
WorkflowDefinition(44-121)
backend/infrahub/core/branch/models.py (1)
backend/infrahub/core/node/standard.py (1)
create(102-116)
backend/infrahub/events/branch_action.py (2)
backend/infrahub/events/models.py (3)
InfrahubEvent(148-200)get_resource(160-161)get_messages(163-164)backend/infrahub/events/schema_action.py (2)
get_resource(30-35)get_messages(37-44)
backend/tests/unit/workflows/test_models.py (1)
backend/infrahub/workflows/models.py (1)
WorkflowParameter(38-41)
backend/infrahub/cli/upgrade.py (2)
backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/cli/db.py (3)
detect_migration_to_run(287-314)migrate_database(317-358)trigger_rebase_branches(361-388)
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py (1)
backend/infrahub/core/migrations/shared.py (3)
MigrationRequiringRebase(227-245)execute_against_branch(239-241)MigrationResult(22-31)
backend/infrahub/core/migrations/graph/__init__.py (1)
backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(212-224)GraphMigration(141-168)InternalSchemaMigration(171-209)MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)
backend/tests/unit/core/migrations/test_runner.py (1)
backend/infrahub/core/migrations/runner.py (2)
MigrationRunner(17-54)has_migrations(35-36)
backend/infrahub/cli/db.py (7)
backend/infrahub/auth.py (2)
AccountSession(44-52)AuthType(38-41)backend/infrahub/context.py (1)
InfrahubContext(33-53)backend/infrahub/core/branch/tasks.py (1)
rebase_branch(107-249)backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/exceptions.py (1)
ValidationError(319-341)backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(212-224)GraphMigration(141-168)InternalSchemaMigration(171-209)MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)backend/infrahub/core/migrations/graph/__init__.py (2)
get_migration_by_number(112-129)get_graph_migrations(99-109)
backend/infrahub/core/branch/tasks.py (6)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workers/dependencies.py (2)
get_event_service(107-108)get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)backend/infrahub/events/models.py (1)
EventMeta(29-145)
🔇 Additional comments (3)
backend/infrahub/task_manager/models.py (1)
146-147: Clarify the counterintuitive condition checking "infrahub.branch.created".The pattern at lines 139-140, 146-147, and 153-154 checks if
"infrahub.branch.created"is NOT in the event_type list before appending branch-specific events (merged/migrated/rebased). This logic lacks documentation and appears semantically inconsistent—why would the absence of a branch creation event determine whether to add branch operation events?Additionally, the GraphQL schema defines only
branch_mergedandbranch_rebasedinEventTypeFilter, but the code also handlesbranch_migrated, suggesting the schema may be incomplete.Either clarify this logic with inline comments or verify the condition should check for the specific event being added rather than
branch.created.schema/schema.graphql (2)
144-150: Field addition looks good.The optional
graph_version: Intfield correctly aligns with the backend model where it's defined as nullable (int | None). Placement among other metadata fields is logical.
324-329: New status enum value is correctly added.The
NEED_UPGRADE_REBASEstatus is appropriately added to the BranchStatus enum as an optional value, consistent with the PR's goal to track branches that require rebase during upgrade.
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py
Outdated
Show resolved
Hide resolved
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
from
2b1294e to
caeba4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
backend/infrahub/core/migrations/exceptions.py (1)
1-4: Add docstrings and improve exception message handling.The previous review comment correctly identified that this exception class is missing required docstrings and doesn't provide a useful exception message when logged or displayed. Please address the feedback in the existing comment.
backend/infrahub/task_manager/models.py (1)
144-149: Address resource filter overwriting and add type annotation.The previous review comments correctly identified two issues affecting this code:
- The
self.resourceassignment on line 149 will overwrite any previous resource filter set bybranch_mergedor later be overwritten bybranch_rebased, preventing multiple branch event types from being filtered correctly.- The
branchesvariable on line 145 is missing an explicit type annotation for consistency with coding guidelines.Please address the feedback in the existing comments.
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py (1)
450-452: Implement branch-side backfill logic.The previous review comment correctly identified that
execute_against_branchis currently a no-op, which means branch nodes won't get theirdisplay_labelandhuman_friendly_idvalues backfilled during migration. This leaves branch data inconsistent with the default branch. Please address the feedback in the existing comment to implement branch-scoped backfill.backend/infrahub/core/migrations/shared.py (1)
227-245: Document and enforce the abstract base contract.This new base class still lacks the Google-style docstrings mandated for Python code, and the coroutine hooks are not marked abstract, so subclasses are not forced to implement them. Please bring it in line with the coding guidelines by making it an abstract base, adding the required docstrings, and decorating the hook methods with
@abstractmethod. Suggested change:+from abc import ABC, abstractmethod @@ -class MigrationRequiringRebase(BaseModel): - model_config = ConfigDict(arbitrary_types_allowed=True) - name: str = Field(..., description="Name of the migration") - minimum_version: int = Field(..., description="Minimum version of the graph to execute this migration") - - @classmethod - def init(cls, **kwargs: dict[str, Any]) -> Self: - return cls(**kwargs) # type: ignore[arg-type] - - async def validate_migration(self, db: InfrahubDatabase) -> MigrationResult: - raise NotImplementedError() - - async def execute_against_branch(self, db: InfrahubDatabase, branch: Branch) -> MigrationResult: - """Method that will be run against non-default branches, it assumes that the branches have been rebased.""" - raise NotImplementedError() - - async def execute(self, db: InfrahubDatabase) -> MigrationResult: - """Method that will be run against the default branch.""" - raise NotImplementedError() +class MigrationRequiringRebase(BaseModel, ABC): + """Base class for migrations that must re-run on rebased branches.""" + + model_config = ConfigDict(arbitrary_types_allowed=True) + name: str = Field(..., description="Name of the migration") + minimum_version: int = Field(..., description="Minimum version of the graph to execute this migration") + + @classmethod + def init(cls, **kwargs: dict[str, Any]) -> Self: + """Instantiate the migration with the provided configuration.""" + return cls(**kwargs) # type: ignore[arg-type] + + @abstractmethod + async def validate_migration(self, db: InfrahubDatabase) -> MigrationResult: + """Validate that the migration completed successfully on the default branch.""" + raise NotImplementedError() + + @abstractmethod + async def execute_against_branch(self, db: InfrahubDatabase, branch: Branch) -> MigrationResult: + """Run the migration against a non-default branch that has just been rebased.""" + raise NotImplementedError() + + @abstractmethod + async def execute(self, db: InfrahubDatabase) -> MigrationResult: + """Run the migration against the default branch during the main upgrade flow.""" + raise NotImplementedError()As per coding guidelines.
backend/infrahub/core/branch/tasks.py (1)
88-90: Propagate migration failures to callers.Swallowing
MigrationFailureErrorlogs the problem but lets the flow finish successfully, sotrigger_rebase_branches(and the CLI--rebase-branchesflag) report success while the branch stays stuck inNEED_UPGRADE_REBASE. Please re-raise after logging so the caller can abort and surface the failure.try: log.info(f"Running migrations for branch '{obj.name}'") await migration_runner.run(db=db) except MigrationFailureError as exc: log.error(f"Failed to run migrations for branch '{obj.name}': {exc.errors}") - return + raise
🧹 Nitpick comments (1)
backend/infrahub/cli/db.py (1)
326-329: Docstring still documents the removed parameter
migrate_databaseno longer acceptsmigration_number, but the Args section still mentions it. Please refresh the docstring to match the current signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/infrahub/cli/db.py(7 hunks)backend/infrahub/cli/upgrade.py(6 hunks)backend/infrahub/core/branch/enums.py(1 hunks)backend/infrahub/core/branch/models.py(3 hunks)backend/infrahub/core/branch/tasks.py(5 hunks)backend/infrahub/core/constants/__init__.py(1 hunks)backend/infrahub/core/migrations/exceptions.py(1 hunks)backend/infrahub/core/migrations/graph/__init__.py(3 hunks)backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py(3 hunks)backend/infrahub/core/migrations/runner.py(1 hunks)backend/infrahub/core/migrations/shared.py(2 hunks)backend/infrahub/events/branch_action.py(2 hunks)backend/infrahub/graphql/types/branch.py(2 hunks)backend/infrahub/task_manager/event.py(2 hunks)backend/infrahub/task_manager/models.py(1 hunks)backend/infrahub/workflows/catalogue.py(2 hunks)backend/tests/unit/core/migrations/test_runner.py(1 hunks)backend/tests/unit/workflows/test_models.py(1 hunks)docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx(1 hunks)docs/docs/reference/infrahub-events.mdx(2 hunks)schema/schema.graphql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/infrahub/events/branch_action.py
- docs/docs/reference/infrahub-events.mdx
- backend/infrahub/workflows/catalogue.py
- backend/infrahub/task_manager/event.py
- docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx
- backend/tests/unit/core/migrations/test_runner.py
- backend/infrahub/core/branch/models.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.pybackend/infrahub/cli/upgrade.pybackend/infrahub/task_manager/models.pybackend/tests/unit/workflows/test_models.pybackend/infrahub/cli/db.pybackend/infrahub/graphql/types/branch.pybackend/infrahub/core/migrations/graph/__init__.pybackend/infrahub/core/constants/__init__.pybackend/infrahub/core/branch/enums.pybackend/infrahub/core/migrations/exceptions.pybackend/infrahub/core/migrations/shared.pybackend/infrahub/core/migrations/runner.pybackend/infrahub/core/branch/tasks.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.pybackend/infrahub/cli/upgrade.pybackend/infrahub/task_manager/models.pybackend/tests/unit/workflows/test_models.pybackend/infrahub/cli/db.pybackend/infrahub/graphql/types/branch.pybackend/infrahub/core/migrations/graph/__init__.pybackend/infrahub/core/constants/__init__.pybackend/infrahub/core/branch/enums.pybackend/infrahub/core/migrations/exceptions.pybackend/infrahub/core/migrations/shared.pybackend/infrahub/core/migrations/runner.pybackend/infrahub/core/branch/tasks.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/workflows/test_models.py
🧬 Code graph analysis (8)
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py (1)
backend/infrahub/core/migrations/shared.py (3)
MigrationRequiringRebase(227-245)execute_against_branch(239-241)MigrationResult(22-31)
backend/infrahub/cli/upgrade.py (2)
backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/cli/db.py (3)
detect_migration_to_run(287-314)migrate_database(317-358)trigger_rebase_branches(361-388)
backend/tests/unit/workflows/test_models.py (1)
backend/infrahub/workflows/models.py (1)
WorkflowParameter(38-41)
backend/infrahub/cli/db.py (7)
backend/infrahub/auth.py (2)
AccountSession(44-52)AuthType(38-41)backend/infrahub/context.py (1)
InfrahubContext(33-53)backend/infrahub/core/branch/tasks.py (1)
rebase_branch(107-249)backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/exceptions.py (1)
ValidationError(319-341)backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(212-224)GraphMigration(141-168)InternalSchemaMigration(171-209)MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)backend/infrahub/core/migrations/graph/__init__.py (2)
get_migration_by_number(112-129)get_graph_migrations(99-109)
backend/infrahub/core/migrations/graph/__init__.py (1)
backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(212-224)GraphMigration(141-168)InternalSchemaMigration(171-209)MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)
backend/infrahub/core/migrations/shared.py (5)
backend/infrahub/core/protocols_base.py (3)
init(77-83)InfrahubDatabase(35-65)Branch(31-31)backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py (3)
validate_migration(373-374)execute_against_branch(450-451)execute(453-493)backend/infrahub/core/migrations/schema/attribute_supports_profile.py (1)
execute(70-90)backend/infrahub/core/migrations/schema/attribute_kind_update.py (1)
execute(150-162)backend/infrahub/core/migrations/schema/node_attribute_add.py (1)
execute(62-71)
backend/infrahub/core/migrations/runner.py (2)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/shared.py (11)
MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)execute_against_branch(239-241)success(27-31)validate_migration(151-152)validate_migration(193-194)validate_migration(220-221)validate_migration(236-237)
backend/infrahub/core/branch/tasks.py (6)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workflows/utils.py (1)
add_tags(22-48)backend/infrahub/workers/dependencies.py (3)
get_database(68-69)get_event_service(107-108)get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: validate-generated-documentation
- GitHub Check: documentation
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
backend/tests/unit/workflows/test_models.py (1)
9-9: LGTM!The new
send_eventsparameter is correctly defined as an optional boolean parameter, consistent with the existing parameter patterns.backend/infrahub/core/branch/enums.py (1)
7-7: LGTM!The new
NEED_UPGRADE_REBASEstatus is clearly named and logically positioned to support the branch migration workflow.backend/infrahub/core/constants/__init__.py (1)
53-53: LGTM!The new
BRANCH_MIGRATEDevent type follows the established naming convention and is logically positioned with other branch-related events.schema/schema.graphql (1)
144-150: Thegraph_versionfield is properly nullable and correctly handled throughout the stack.The verification confirms:
- Backend model (
core/branch/models.py:48): defined asint | None = None✓- GraphQL resolver (
graphql/types/branch.py:24): exposed asInt(required=False)✓- Proper initialization and versioning logic exists in the codebase to handle both new and existing branches
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
from
0047cf4 to
caeba4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/infrahub/core/branch/needs_rebase_status.py (2)
5-6: Add docstring to document the function.The coding guidelines require Google-style docstrings for all Python functions. Consider adding documentation to clarify the purpose and parameters.
Apply this diff:
def raise_needs_rebase_error(branch_name: str) -> None: + """Raise an error indicating the branch must be rebased before updates. + + Args: + branch_name: Name of the branch requiring rebase. + + Raises: + ValueError: Always raised to prevent updates on branches needing rebase. + """ raise ValueError(f"Branch {branch_name} must be rebased before any updates can be made")As per coding guidelines.
9-11: Add docstring to document the function.The coding guidelines require Google-style docstrings for all Python functions. Consider adding documentation to clarify the purpose, parameters, and behavior.
Apply this diff:
def check_need_rebase_status(branch: Branch) -> None: + """Check if a branch requires rebase and raise an error if so. + + Verifies whether the branch has a status indicating rebase is needed + (either standard rebase or upgrade-rebase) and raises an error to + prevent updates. + + Args: + branch: The branch to check for rebase requirements. + + Raises: + ValueError: If the branch has NEED_REBASE or NEED_UPGRADE_REBASE status. + """ if branch.status in (BranchStatus.NEED_REBASE, BranchStatus.NEED_UPGRADE_REBASE): raise_needs_rebase_error(branch_name=branch.name)As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/core/branch/needs_rebase_status.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/branch/needs_rebase_status.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/branch/needs_rebase_status.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
📚 Learning: 2025-10-30T12:43:45.629Z
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
backend/infrahub/core/branch/needs_rebase_status.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: validate-generated-documentation
- GitHub Check: documentation
- GitHub Check: backend-tests-integration
🔇 Additional comments (1)
backend/infrahub/core/branch/needs_rebase_status.py (1)
10-10: LGTM! Correct handling of both rebase statuses.The expanded condition properly includes both
NEED_REBASEandNEED_UPGRADE_REBASE, ensuring upgrade-rebase scenarios trigger the same error path. The tuple-based membership check is idiomatic Python.
| return start, end | ||
|
|
||
| async def create(self, db: InfrahubDatabase) -> bool: | ||
| self.graph_version = GRAPH_VERSION |
There was a problem hiding this comment.
One question here is how we manage this number after a rebase operation. Do we then bump the graph version? (I haven't yet checked if we already do this, just commenting some thoughts)
There was a problem hiding this comment.
This number only changes in the branch-migrate flow, if there are no migrations to apply or if migrations were applied succesfully. This means that if a branch is created at graph version n and the global graph version is changed to m, the branch's graph version will never be m unless a rebase operation happens.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/infrahub/core/branch/tasks.py (1)
87-92: Re-raise migration failures to prevent silent errors.Catching
MigrationFailureErrorand returning silently leaves the branch inNEED_UPGRADE_REBASEstatus while the caller (e.g.,trigger_rebase_branchesin the CLI) cannot detect the failure. This can mislead users into thinking the migration succeeded. The exception should propagate so callers can handle it appropriately.Apply this diff to re-raise after logging:
try: log.info(f"Running migrations for branch '{obj.name}'") await migration_runner.run(db=db) except MigrationFailureError as exc: log.error(f"Failed to run migrations for branch '{obj.name}': {exc.errors}") - return + raise
🧹 Nitpick comments (1)
backend/infrahub/core/branch/tasks.py (1)
72-74: Make the log message more explicit about what is up-to-date.The current message "no migrations to apply" could be more specific about what aspect is up-to-date (the graph version/structure).
Apply this diff:
- if obj.graph_version == GRAPH_VERSION: - log.info(f"Branch '{obj.name}' has graph version {obj.graph_version}, no migrations to apply") - return + if obj.graph_version == GRAPH_VERSION: + log.info(f"Branch '{obj.name}' graph version matches the expected version {GRAPH_VERSION}, no migrations required") + returnBased on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/core/branch/tasks.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/branch/tasks.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/branch/tasks.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
📚 Learning: 2025-10-30T12:43:45.629Z
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
backend/infrahub/core/branch/tasks.py
🧬 Code graph analysis (1)
backend/infrahub/core/branch/tasks.py (6)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workflows/utils.py (1)
add_tags(22-48)backend/infrahub/workers/dependencies.py (3)
get_database(68-69)get_event_service(107-108)get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: documentation
- GitHub Check: validate-generated-documentation
🔇 Additional comments (2)
backend/infrahub/core/branch/tasks.py (2)
110-110: Good practice: centralized workflow retrieval.Creating a local
workflowvariable and reusing it throughout the function (lines 129, 216, 223) is cleaner than callingget_workflow()multiple times. This improves consistency and readability.
222-226: Post-rebase migration integration looks correct.The flow correctly:
- Calls
migrate_branchto apply branch-specific migrations after the rebase- Submits the
DIFF_REFRESH_ALLworkflow to update diffs- Propagates the
send_eventsparameter to control event emissionThe early return at line 226 prevents duplicate event processing when
send_events=False.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/infrahub/core/branch/tasks.py (2)
62-106: Add a Google-style docstring.The function is missing a docstring. Per coding guidelines, all Python functions should have a Google-style docstring with a brief description, Args section (without types), and Returns/Raises sections.
Example docstring:
@flow(name="branch-migrate", flow_run_name="Apply migrations to branch {branch}") async def migrate_branch(branch: str, context: InfrahubContext, send_events: bool = True) -> None: """Apply migrations to a branch and update its graph version. This flow checks if a branch requires migrations based on its graph_version, applies any pending migrations via MigrationRunner, updates the branch status, and optionally emits a BranchMigratedEvent upon successful completion. Args: branch: Name of the branch to migrate. context: Infrahub context for the operation. send_events: Whether to emit events after successful migration. Defaults to True. Raises: BranchNotFoundError: If the specified branch does not exist. MigrationFailureError: If any migration fails execution or validation. """As per coding guidelines.
108-109: Update the docstring to document the new parameter.The
send_eventsparameter has been added but the function lacks a docstring documenting this parameter and the function's behavior. Per coding guidelines, this function needs a Google-style docstring.Add a docstring like:
@flow(name="branch-rebase", flow_run_name="Rebase branch {branch}") async def rebase_branch(branch: str, context: InfrahubContext, send_events: bool = True) -> None: """Rebase a branch onto the default branch and apply post-rebase migrations. This flow performs conflict detection, schema validation, rebases the branch, applies schema migrations, triggers IPAM reconciliation, runs branch migrations, refreshes diffs, and optionally emits rebase and node change events. Args: branch: Name of the branch to rebase. context: Infrahub context for the operation. send_events: Whether to emit events after successful rebase. Defaults to True. Raises: ValidationError: If conflicts exist or validation constraints fail. BranchNotFoundError: If the specified branch does not exist. """As per coding guidelines.
backend/infrahub/cli/db.py (1)
288-315: Addcheckparameter to support re-running migrations.When a user invokes
infrahub db migrate --migration-number <N>without--check, the function still returns an empty list whencurrent_graph_version > minimum_version(lines 298-302). This breaks the documented ability to re-run a migration on demand. The message on line 300 instructs users to "run without the --check flag," but the function doesn't receive acheckparameter to distinguish between checking status vs. actually re-running a migration.Thread the
checkflag through from the caller and only return early whencheck=True:async def detect_migration_to_run( - current_graph_version: int, migration_number: int | str | None = None + current_graph_version: int, + migration_number: int | str | None = None, + *, + check: bool = False, ) -> Sequence[GraphMigration | InternalSchemaMigration | ArbitraryMigration | MigrationRequiringRebase]:if migration_number: migration = get_migration_by_number(migration_number) - migrations.append(migration) if current_graph_version > migration.minimum_version: rprint( f"Migration {migration_number} already applied. To apply again, run the command without the --check flag." ) - return [] + if check: + return [] + migrations.append(migration)Then update the caller in
migrate_cmd:migrations = await detect_migration_to_run( - current_graph_version=root_node.graph_version, migration_number=migration_number + current_graph_version=root_node.graph_version, + migration_number=migration_number, + check=check, )
🧹 Nitpick comments (2)
backend/infrahub/cli/db.py (2)
382-384: Consider using an authenticated system account for background operations.The
InfrahubContextis created with an unauthenticatedAccountSessionwith an emptyaccount_id. For system-level operations like automated branch rebasing and migrations, consider using a dedicated system account to properly track and audit these operations.If a system account exists or can be created for background operations, use it instead:
- context=InfrahubContext.init( - branch=branch, account=AccountSession(auth_type=AuthType.NONE, authenticated=False, account_id="") - ), + context=InfrahubContext.init( + branch=branch, + account=AccountSession( + auth_type=AuthType.API, + authenticated=True, + account_id=config.SETTINGS.system.internal_account_id + ) + ),
362-389: Track and report overall rebase failure status.The function catches exceptions for individual branch rebases but continues processing remaining branches without tracking whether any failures occurred. The caller (e.g., the upgrade CLI) has no way to determine if all rebases succeeded or if some failed.
Track failures and return or raise an indication of partial failure:
async def trigger_rebase_branches(db: InfrahubDatabase) -> None: """Trigger rebase of non-default branches, also triggering migrations in the process.""" branches = [b for b in await Branch.get_list(db=db) if b.name not in [registry.default_branch, GLOBAL_BRANCH_NAME]] if not branches: return rprint(f"Planning rebase and migrations for {len(branches)} branches: {', '.join([b.name for b in branches])}") + failed_branches: list[str] = [] for branch in branches: if branch.graph_version == GRAPH_VERSION: rprint(...) continue rprint(f"Rebasing branch '{branch.name}' (ID: {branch.uuid})...", end="") try: await registry.schema.load_schema(db=db, branch=branch) await rebase_branch(...) rprint(SUCCESS_BADGE) except (ValidationError, MigrationFailureError): rprint(FAILED_BADGE) + failed_branches.append(branch.name) + + if failed_branches: + raise RuntimeError(f"Failed to rebase branches: {', '.join(failed_branches)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/cli/db.py(7 hunks)backend/infrahub/core/branch/tasks.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/branch/tasks.pybackend/infrahub/cli/db.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/branch/tasks.pybackend/infrahub/cli/db.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
📚 Learning: 2025-10-30T12:43:45.629Z
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
backend/infrahub/core/branch/tasks.py
🧬 Code graph analysis (2)
backend/infrahub/core/branch/tasks.py (6)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workflows/utils.py (1)
add_tags(22-48)backend/infrahub/workers/dependencies.py (3)
get_database(68-69)get_event_service(107-108)get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)
backend/infrahub/cli/db.py (8)
backend/infrahub/auth.py (2)
AccountSession(44-52)AuthType(38-41)backend/infrahub/context.py (1)
InfrahubContext(33-53)backend/infrahub/core/branch/tasks.py (1)
rebase_branch(109-251)backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/exceptions.py (1)
ValidationError(319-341)backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(212-224)GraphMigration(141-168)InternalSchemaMigration(171-209)MigrationRequiringRebase(227-245)init(148-149)init(190-191)init(217-218)init(233-234)backend/infrahub/core/migrations/graph/__init__.py (2)
get_migration_by_number(112-129)get_graph_migrations(99-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: validate-generated-documentation
- GitHub Check: documentation
- GitHub Check: backend-tests-unit
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
2 times, most recently
from
c808995 to
ec95310
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
backend/infrahub/task_manager/models.py (2)
144-149: Resource overwriting issue persists in new branch_migrated handler.The new
branch_migratedevent handler (line 149) follows the same problematic pattern already flagged in past reviews: it assignsself.resourcedirectly, which overwrites any resource filter set by earlier blocks (e.g.,branch_mergedat line 142). If multiple branch event types are specified, only the last one's resource filter will be retained.The previous review recommended consolidating resource filters using a method that appends/combines them (similar to
add_related_filter) rather than direct assignment. This fix should apply to the newbranch_migratedblock as well as the existingbranch_merged,branch_rebased, andadd_primary_node_filtermethods.
145-145: Missing type annotation for consistency.The
branchesvariable on line 145 is missing the explicit type annotation present on line 138 for thebranch_mergedcase. Per coding guidelines requiring type hints, add the annotation for consistency.Apply this diff:
- if branch_migrated := event_type_filter.get("branch_migrated"): - branches = branch_migrated.get("branches") or [] + if branch_migrated := event_type_filter.get("branch_migrated"): + branches: list[str] = branch_migrated.get("branches") or []backend/infrahub/core/migrations/exceptions.py (1)
1-4: Add docstrings and construct a meaningful exception message.The exception class lacks docstrings and doesn't provide a useful message when raised or logged.
Apply this diff:
class MigrationFailureError(Exception): + """Exception raised when a migration fails to execute or validate. + + Attributes: + errors: List of error messages from the failed migration + """ def __init__(self, errors: list[str]) -> None: - super().__init__() + """Initialize the exception with migration errors. + + Args: + errors: List of error messages describing the migration failure + """ + message = f"Migration failed with {len(errors)} error(s): {'; '.join(errors)}" + super().__init__(message) self.errors = errorsAs per coding guidelines.
backend/infrahub/core/migrations/shared.py (1)
229-247: Add docstrings and @AbstractMethod decorators.The class and its methods lack required docstrings, and the abstract methods should be explicitly decorated to enforce implementation in subclasses.
Apply this diff:
+from abc import abstractmethod + class MigrationRequiringRebase(BaseModel): + """Base class for migrations that require branch rebases. + + Migrations subclassing this type define separate execution paths for + the default branch and for non-default branches after rebase. + """ model_config = ConfigDict(arbitrary_types_allowed=True) name: str = Field(..., description="Name of the migration") minimum_version: int = Field(..., description="Minimum version of the graph to execute this migration") @classmethod def init(cls, **kwargs: dict[str, Any]) -> Self: + """Initialize the migration instance. + + Args: + **kwargs: Migration configuration parameters + + Returns: + Initialized migration instance + """ return cls(**kwargs) # type: ignore[arg-type] + @abstractmethod async def validate_migration(self, db: InfrahubDatabase) -> MigrationResult: + """Validate whether the migration can be applied. + + Args: + db: Database connection + + Returns: + Validation result with any errors + """ raise NotImplementedError() + @abstractmethod async def execute_against_branch(self, db: InfrahubDatabase, branch: Branch) -> MigrationResult: - """Method that will be run against non-default branches, it assumes that the branches have been rebased.""" + """Execute the migration against a non-default branch after rebase. + + Args: + db: Database connection + branch: The branch to migrate (must be rebased) + + Returns: + Migration result with any errors + """ raise NotImplementedError() + @abstractmethod async def execute(self, db: InfrahubDatabase) -> MigrationResult: - """Method that will be run against the default branch.""" + """Execute the migration against the default branch. + + Args: + db: Database connection + + Returns: + Migration result with any errors + """ raise NotImplementedError()As per coding guidelines.
backend/infrahub/core/branch/tasks.py (2)
62-106: Add docstring and consider DB session wrapping.The new
migrate_branchflow correctly handles migration detection, status updates, and event emission, but it lacks a required docstring and should wrap the entire operation in a single database session for consistency.Add a docstring:
@flow(name="branch-migrate", flow_run_name="Apply migrations to branch {branch}") async def migrate_branch(branch: str, context: InfrahubContext, send_events: bool = True) -> None: + """Apply migrations to a branch and update its graph version. + + This flow checks if a branch requires migrations based on its graph_version, + applies any pending migrations via MigrationRunner, updates the branch status, + and optionally emits a BranchMigratedEvent upon successful completion. + + Args: + branch: Name of the branch to migrate + context: Infrahub context for the operation + send_events: Whether to emit events after successful migration + + Raises: + BranchNotFoundError: If the specified branch does not exist + MigrationFailureError: If migration execution or validation fails + """ await add_tags(branches=[branch])As per coding guidelines.
109-109: Add docstring to document the new send_events parameter.The
rebase_branchflow has been updated with asend_eventsparameter, but the function lacks a docstring documenting this change and the overall behavior.Add a docstring:
@flow(name="branch-rebase", flow_run_name="Rebase branch {branch}") async def rebase_branch(branch: str, context: InfrahubContext, send_events: bool = True) -> None: """Rebase a branch onto the default branch and apply post-rebase migrations. This flow performs conflict detection, schema validation, rebases the branch, applies schema migrations, triggers IPAM reconciliation, runs branch migrations, refreshes diffs, and optionally emits rebase and node change events. Args: branch: Name of the branch to rebase context: Infrahub context for the operation send_events: Whether to emit events after successful rebase Raises: ValidationError: If conflicts exist or validation constraints fail BranchNotFoundError: If the specified branch does not exist """As per coding guidelines.
Also applies to: 222-226
backend/infrahub/cli/db.py (2)
117-125: Keep explicit migrations runnable when not using--check.Requesting a specific migration (e.g.
infrahub db migrate --migration-number …without--check) still hits the earlyreturn []oncecurrent_graph_version > minimum_version. The CLI prints “run without --check” even though we already did, so the migration never executes—exactly the regression called out earlier. Please thread thecheckflag throughdetect_migration_to_runand only drop the migration whencheckis true.- migrations = await detect_migration_to_run( - current_graph_version=root_node.graph_version, migration_number=migration_number - ) + migrations = await detect_migration_to_run( + current_graph_version=root_node.graph_version, + migration_number=migration_number, + check=check, + ) @@ -async def detect_migration_to_run( - current_graph_version: int, migration_number: int | str | None = None +async def detect_migration_to_run( + current_graph_version: int, + migration_number: int | str | None = None, + *, + check: bool = False, ) -> Sequence[GraphMigration | InternalSchemaMigration | ArbitraryMigration | MigrationRequiringRebase]: @@ - migrations.append(migration) if current_graph_version > migration.minimum_version: rprint( f"Migration {migration_number} already applied. To apply again, run the command without the --check flag." ) - return [] + if check: + return [] + migrations.append(migration)Also applies to: 295-307
364-389: Open a DB session before loading branch schemas.
registry.schema.load_schemais still invoked without anasync with db.start_session()guard, so the per-branch work runs outside a managed session—the same issue previously flagged. Wrap each branch iteration in a session and pass it toload_schemabefore rebasing.for branch in branches: if branch.graph_version == GRAPH_VERSION: rprint( f"Ignoring branch rebase and migrations for '{branch.name}' (ID: {branch.uuid}), it is already up-to-date" ) continue rprint(f"Rebasing branch '{branch.name}' (ID: {branch.uuid})...", end="") try: - await registry.schema.load_schema(db=db, branch=branch) - await rebase_branch( - branch=branch.name, - context=InfrahubContext.init( - branch=branch, account=AccountSession(auth_type=AuthType.NONE, authenticated=False, account_id="") - ), - send_events=False, - ) + async with db.start_session() as db_session: + await registry.schema.load_schema(db=db_session, branch=branch) + await rebase_branch( + branch=branch.name, + context=InfrahubContext.init( + branch=branch, + account=AccountSession(auth_type=AuthType.NONE, authenticated=False, account_id=""), + ), + send_events=False, + ) rprint(SUCCESS_BADGE) except (ValidationError, MigrationFailureError): rprint(FAILED_BADGE)
🧹 Nitpick comments (2)
backend/infrahub/core/branch/tasks.py (2)
72-74: Consider clarifying the log message.The current message "no migrations to apply" could be more explicit about what "up to date" means in this context.
Apply this diff to improve clarity:
- log.info(f"Branch '{obj.name}' has graph version {obj.graph_version}, no migrations to apply") + log.info(f"Branch '{obj.name}' graph version {obj.graph_version} matches expected version {GRAPH_VERSION}, no migrations to apply")Based on learnings from ogenstad's comment about verbosity.
83-85: Optimize branch status update to avoid unnecessary save.If the branch status is already
NEED_UPGRADE_REBASE, the save operation is redundant. Check the current status before updating to avoid an unnecessary database write.Apply this diff:
- if obj.status != BranchStatus.NEED_UPGRADE_REBASE: - obj.status = BranchStatus.NEED_UPGRADE_REBASE - await obj.save(db=db) + if obj.status != BranchStatus.NEED_UPGRADE_REBASE: + obj.status = BranchStatus.NEED_UPGRADE_REBASE + await obj.save(db=db)Wait, the code already checks
if obj.status != BranchStatus.NEED_UPGRADE_REBASE:before saving. This is already optimized!Actually, looking at the code again, it already has the check. Let me re-read ogenstad's comment... They're saying "we only do that provided that the status is something else" - which is exactly what the code does. This is already correct, so I should approve instead.
Actually, I should mark this as a non-issue or approve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
backend/infrahub/cli/db.py(7 hunks)backend/infrahub/cli/upgrade.py(6 hunks)backend/infrahub/core/branch/enums.py(1 hunks)backend/infrahub/core/branch/models.py(3 hunks)backend/infrahub/core/branch/tasks.py(5 hunks)backend/infrahub/core/constants/__init__.py(1 hunks)backend/infrahub/core/migrations/exceptions.py(1 hunks)backend/infrahub/core/migrations/graph/__init__.py(3 hunks)backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py(2 hunks)backend/infrahub/core/migrations/runner.py(1 hunks)backend/infrahub/core/migrations/shared.py(2 hunks)backend/infrahub/events/branch_action.py(2 hunks)backend/infrahub/graphql/types/branch.py(2 hunks)backend/infrahub/task_manager/event.py(2 hunks)backend/infrahub/task_manager/models.py(1 hunks)backend/infrahub/workflows/catalogue.py(2 hunks)backend/tests/unit/core/migrations/test_runner.py(1 hunks)backend/tests/unit/workflows/test_models.py(1 hunks)docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx(1 hunks)docs/docs/reference/infrahub-events.mdx(2 hunks)schema/schema.graphql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/tests/unit/workflows/test_models.py
- backend/infrahub/core/constants/init.py
- docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx
- docs/docs/reference/infrahub-events.mdx
- backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py
- backend/infrahub/graphql/types/branch.py
- backend/infrahub/core/migrations/runner.py
- backend/tests/unit/core/migrations/test_runner.py
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/task_manager/event.pybackend/infrahub/core/migrations/shared.pybackend/infrahub/events/branch_action.pybackend/infrahub/task_manager/models.pybackend/infrahub/core/branch/enums.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/migrations/exceptions.pybackend/infrahub/cli/db.pybackend/infrahub/cli/upgrade.pybackend/infrahub/core/migrations/graph/__init__.pybackend/infrahub/core/branch/tasks.pybackend/infrahub/core/branch/models.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/task_manager/event.pybackend/infrahub/core/migrations/shared.pybackend/infrahub/events/branch_action.pybackend/infrahub/task_manager/models.pybackend/infrahub/core/branch/enums.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/migrations/exceptions.pybackend/infrahub/cli/db.pybackend/infrahub/cli/upgrade.pybackend/infrahub/core/migrations/graph/__init__.pybackend/infrahub/core/branch/tasks.pybackend/infrahub/core/branch/models.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
📚 Learning: 2025-10-30T12:43:45.629Z
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
schema/schema.graphqlbackend/infrahub/core/branch/enums.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/branch/tasks.pybackend/infrahub/core/branch/models.py
🧬 Code graph analysis (8)
backend/infrahub/core/migrations/shared.py (1)
backend/infrahub/core/protocols_base.py (2)
InfrahubDatabase(35-65)Branch(31-31)
backend/infrahub/events/branch_action.py (3)
backend/infrahub/events/models.py (3)
InfrahubEvent(148-200)get_resource(160-161)get_messages(163-164)backend/infrahub/events/group_action.py (1)
get_resource(85-93)backend/infrahub/events/schema_action.py (2)
get_resource(30-35)get_messages(37-44)
backend/infrahub/workflows/catalogue.py (1)
backend/infrahub/workflows/models.py (1)
WorkflowDefinition(44-121)
backend/infrahub/cli/db.py (7)
backend/infrahub/auth.py (2)
AccountSession(44-52)AuthType(38-41)backend/infrahub/context.py (1)
InfrahubContext(33-53)backend/infrahub/core/branch/tasks.py (1)
rebase_branch(109-251)backend/infrahub/exceptions.py (1)
ValidationError(319-341)backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(214-226)GraphMigration(143-170)InternalSchemaMigration(173-211)MigrationRequiringRebase(229-247)init(150-151)init(192-193)init(219-220)init(235-236)backend/infrahub/core/migrations/graph/__init__.py (2)
get_migration_by_number(112-129)get_graph_migrations(99-109)backend/infrahub/core/schema/manager.py (1)
load_schema(618-644)
backend/infrahub/cli/upgrade.py (2)
backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/cli/db.py (4)
detect_migration_to_run(288-315)initialize_internal_schema(392-395)migrate_database(318-359)trigger_rebase_branches(362-389)
backend/infrahub/core/migrations/graph/__init__.py (1)
backend/infrahub/core/migrations/shared.py (8)
ArbitraryMigration(214-226)GraphMigration(143-170)InternalSchemaMigration(173-211)MigrationRequiringRebase(229-247)init(150-151)init(192-193)init(219-220)init(235-236)
backend/infrahub/core/branch/tasks.py (6)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workflows/utils.py (1)
add_tags(22-48)backend/infrahub/workers/dependencies.py (3)
get_database(68-69)get_event_service(107-108)get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)
backend/infrahub/core/branch/models.py (1)
backend/infrahub/core/node/standard.py (1)
create(102-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: documentation
- GitHub Check: Cloudflare Pages
🔇 Additional comments (24)
backend/infrahub/workflows/catalogue.py (2)
233-239: LGTM! Workflow definition is correctly structured.The BRANCH_MIGRATE workflow definition follows the established pattern and is appropriately configured:
- Uses
WorkflowType.COREconsistent with other branch operations- Includes
DATABASE_CHANGEtag, which is appropriate for migration operations- Points to the correct module and function (
infrahub.core.branch.tasks:migrate_branch)- Matches the pattern of similar workflows like
BRANCH_REBASE
651-651: LGTM! Correct alphabetical placement.The workflow is correctly inserted in alphabetical order between
BRANCH_MERGE_POST_PROCESSandBRANCH_REBASE, maintaining consistency with the rest of theWORKFLOWSlist.backend/infrahub/core/branch/enums.py (1)
7-7: LGTM! Clean enum addition.The new
NEED_UPGRADE_REBASEstatus follows the existing naming conventions and integrates well with the migration/rebase workflow. Based on learnings, ensure that branches with this status have conflict resolution capabilities implemented in the broader codebase.schema/schema.graphql (2)
147-147: Approve: graph_version field addition.The new nullable
Intfield is correctly positioned and will track the graph structure version of branches. The nullable type is appropriate since not all branches may have this value immediately.Please verify that the backend Type resolver and database model properly expose and persist the
graph_versionfield as described in the PR objectives.
327-327: Approve: NEED_UPGRADE_REBASE enum value added to BranchStatus.The new enum value follows existing naming conventions and is correctly positioned in the enum. This status enables the migration-with-rebase workflow described in the PR.
Please verify that:
- The backend properly sets and transitions branches to
NEED_UPGRADE_REBASEstatus during the migration process.- Conflict resolution capabilities are fully supported for branches in this status (as noted in learnings: branches with
NEED_UPGRADE_REBASEmust support conflict resolution).backend/infrahub/core/branch/models.py (2)
9-10: LGTM: Import consolidation and new constant.The import changes cleanly consolidate
GLOBAL_BRANCH_NAMEto a single line and add theGRAPH_VERSIONconstant needed for the new versioning logic.
48-48: LGTM: Graph version field added to track migration state.The nullable
graph_versionfield appropriately tracks the graph version of each branch, enabling migration detection and rebase workflows introduced elsewhere in this PR.backend/infrahub/task_manager/event.py (2)
163-164: LGTM: Branch migrated event handler follows established pattern.The
_return_branch_migratedmethod correctly follows the same pattern as other branch event handlers (_return_branch_rebased,_return_branch_deleted, etc.), returning a dictionary with the branch name extracted via_get_branch_name_from_resource().
234-235: LGTM: Event routing correctly wired for branch migration events.The case statement properly routes
infrahub.branch.migratedevents to the new handler, maintaining consistency with the event-specific dispatch pattern used throughout_return_event_specifics.backend/infrahub/events/branch_action.py (2)
112-112: LGTM: Improved field description accuracy.The field description change from "The ID of the mutated node" to "The ID of the branch" more accurately describes what
branch_idrepresents in the context of a branch rebase event.
133-156: LGTM: BranchMigratedEvent properly mirrors BranchRebasedEvent.The new event class correctly follows the established pattern for branch events:
- Consistent event naming convention (
{EVENT_NAMESPACE}.branch.migrated)- Same field structure as
BranchRebasedEvent- Proper resource metadata in
get_resource()- Appropriate message emission via
RefreshRegistryRebasedBranchbackend/infrahub/core/migrations/shared.py (1)
11-11: LGTM: Import consolidation.The multi-line import has been cleanly consolidated to a single line with no semantic change.
backend/infrahub/cli/upgrade.py (4)
14-19: LGTM: Import additions support new migration and rebase workflow.The new imports correctly wire in the root node retrieval, migration detection, and branch rebase triggering functionality introduced in this PR.
Also applies to: 34-40
54-54: LGTM: Optional rebase flag provides safe opt-in behavior.The
--rebase-branchesflag defaults toFalse, ensuring existing upgrade workflows are unaffected while allowing users to opt into the new branch migration behavior when ready.
72-72: LGTM: Root node retrieval and migration detection flow correctly implemented.The upgrade flow now:
- Retrieves the root node to access
graph_version- Detects applicable migrations before execution
- Properly closes the database driver on early exit when
--checkis used (addressing the past review concern)Also applies to: 84-89
113-117: LGTM: Post-upgrade rebase trigger properly sequenced.The branch rebase and migration step is correctly placed after all core upgrade steps (database migrations, schema updates, internal object upgrades, and task manager setup), ensuring branches are rebased only after the default branch is fully upgraded.
backend/infrahub/core/branch/tasks.py (4)
83-85: LGTM: Status check prevents unnecessary database write.The code correctly checks if the status is already
NEED_UPGRADE_REBASEbefore saving, avoiding redundant database operations when a branch is already marked as needing upgrade.
90-92: LGTM: Migration failures now properly propagate.The exception is now re-raised after logging (previously it was swallowed with a return), ensuring that CLI callers and orchestrators can detect and handle migration failures appropriately. This addresses the previous concern about silent failures.
110-110: LGTM: Workflow object centralization improves consistency.Replacing multiple
get_workflow()calls with a single localworkflowobject improves consistency and makes the code easier to test and reason about.Also applies to: 129-129, 216-216
222-224: LGTM: Post-rebase migration and diff refresh properly sequenced.The flow correctly:
- Invokes
migrate_branchto apply pending migrations after the rebase- Submits a diff refresh workflow to update the branch's diff state
- Respects the
send_eventsflag for both operationsbackend/infrahub/core/migrations/graph/__init__.py (4)
50-50: LGTM: Import updates reflect new migration type.The TYPE_CHECKING imports correctly remove the now-unused
Rootimport and addMigrationRequiringRebaseto support the expanded migration type system.
52-96: LGTM: Migration list type hint extended.The
MIGRATIONSlist type correctly includesMigrationRequiringRebasein the union, ensuring type safety for migrations that require branch rebases.
99-109: LGTM: Decoupled migration detection improves testability.The signature change from accepting a
Rootnode to acceptingcurrent_graph_version: intimproves the function by:
- Decoupling from database access (no need to pass
Rootordb)- Making the function pure and easier to test
- Clarifying the dependency (only the version number is needed)
The return type is correctly updated to include
MigrationRequiringRebase.
112-129: LGTM: Return type consistently updated.The
get_migration_by_numberreturn type correctly includesMigrationRequiringRebase, maintaining consistency with the updated migration type system.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/infrahub/core/branch/tasks.py (1)
62-64: Document migrate_branch per our Python style.Line 63 adds a new flow but there’s no Google-style docstring covering the summary, Args, Returns, and Raises. Please add one so the behavior and parameters (including send_events) are documented. As per coding guidelines.
@flow(name="branch-migrate", flow_run_name="Apply migrations to branch {branch}") async def migrate_branch(...): """Apply branch-specific migrations and update state. Args: ... Returns: None Raises: ... """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/core/branch/tasks.py(5 hunks)changelog/+upgrade-process.changed.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
changelog/+upgrade-process.changed.md
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/branch/tasks.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/branch/tasks.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
📚 Learning: 2025-10-30T12:43:45.629Z
Learnt from: ogenstad
PR: opsmill/infrahub#7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.629Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
changelog/+upgrade-process.changed.mdbackend/infrahub/core/branch/tasks.py
🧬 Code graph analysis (1)
backend/infrahub/core/branch/tasks.py (5)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workers/dependencies.py (1)
get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: documentation
- GitHub Check: validate-generated-documentation
| # EventBranchMigrated( | ||
| # branch=self.branch, | ||
| # meta=self.get_message_meta(), | ||
| # ), |
There was a problem hiding this comment.
It's actually not, all the other get_messages methods have this in their bodies, so I put this for consistency. I don't know what the initial intent was.
This is done by instantiating a `MigrationRunner` which will take care of determining what migrations need to run to then run them if any.
gmazoyer
force-pushed
the
backendteam-20251015-migration-with-rebase
branch
from
1449de7 to
d92dfc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/infrahub/core/branch/tasks.py (2)
76-81: Reset branch status when no migrations are needed.Similar to the previous early return, this path (lines 77-81) should reset the branch status from
NEED_UPGRADE_REBASEtoOPENif the branch is already up-to-date and no migrations are needed.Apply this diff:
migration_runner = MigrationRunner(branch=obj) if not migration_runner.has_migrations(): log.info(f"No migrations detected for branch '{obj.name}'") obj.graph_version = GRAPH_VERSION + if obj.status == BranchStatus.NEED_UPGRADE_REBASE: + obj.status = BranchStatus.OPEN await obj.save(db=db) return
62-74: Add Google-style docstring and reset branch status on early return.Two issues:
Missing docstring: Per coding guidelines, all Python functions require Google-style docstrings with Args, Returns, and Raises sections.
Branch stuck in NEED_UPGRADE_REBASE: The early return at lines 72-74 doesn't reset the branch status. If a branch was previously in
NEED_UPGRADE_REBASEstatus and itsgraph_versionis now current, the status should be cleared toOPEN.As per coding guidelines.
Apply these changes:
@flow(name="branch-migrate", flow_run_name="Apply migrations to branch {branch}") async def migrate_branch(branch: str, context: InfrahubContext, send_events: bool = True) -> None: + """Apply migrations to a branch and update its graph version. + + This flow checks if a branch requires migrations based on its graph_version, + applies any pending migrations via MigrationRunner, updates the branch status, + and optionally emits a BranchMigratedEvent upon successful completion. + + Args: + branch: Name of the branch to migrate. + context: Infrahub context for the operation. + send_events: Whether to emit events after successful migration. + + Raises: + BranchNotFoundError: If the specified branch does not exist. + MigrationFailureError: If migration execution or validation fails. + """ await add_tags(branches=[branch]) database = await get_database() async with database.start_session() as db: log = get_run_logger() obj = await Branch.get_by_name(db=db, name=branch) if obj.graph_version == GRAPH_VERSION: + if obj.status == BranchStatus.NEED_UPGRADE_REBASE: + obj.status = BranchStatus.OPEN + await obj.save(db=db) log.info(f"Branch '{obj.name}' has graph version {obj.graph_version}, no migrations to apply") return
🧹 Nitpick comments (5)
backend/tests/unit/core/migrations/test_runner.py (1)
43-59: Hardcoded migration name creates maintenance burden.Lines 53 and 59 hardcode the migration name "043_backfill_hfid_display_label_in_db". As noted in a past review comment, this test will need updating every time a new
MigrationRequiringRebaseis added, making it brittle.Consider refactoring to make the test more resilient:
- Assert that
applicable_migrationsis non-empty and contains instances ofMigrationRequiringRebase- Or use the first migration in the list dynamically instead of asserting a specific name
Example refactor:
branch.graph_version = None runner = MigrationRunner(branch=branch) await branch.save(db=db) assert runner.applicable_migrations - assert [m.name for m in runner.applicable_migrations][0] == "043_backfill_hfid_display_label_in_db" + assert all(isinstance(m, MigrationRequiringRebase) for m in runner.applicable_migrations) + # Verify we get at least one migration when graph_version is None + assert len(runner.applicable_migrations) > 0backend/infrahub/cli/upgrade.py (1)
49-55: Add comprehensive Google-style docstring.The function lacks a complete docstring. Per coding guidelines, Python functions should have Google-style docstrings with Args, Returns, and Raises sections.
As per coding guidelines.
Example docstring:
async def upgrade_cmd( ctx: typer.Context, config_file: str = typer.Argument("infrahub.toml", envvar="INFRAHUB_CONFIG"), check: bool = typer.Option(False, help="Check the state of the system without upgrading."), rebase_branches: bool = typer.Option(False, help="Rebase and apply migrations to branches if required."), ) -> None: """Upgrade Infrahub to the latest version. This command performs a full upgrade of Infrahub including database migrations, schema updates, internal object updates, and optionally rebases branches to apply branch-specific migrations. Args: ctx: Typer context object. config_file: Path to the Infrahub configuration file. check: If True, only check for pending migrations without applying them. rebase_branches: If True, rebase and migrate all non-default branches after the upgrade. Raises: typer.Exit: If migrations fail during the upgrade process. """backend/infrahub/core/branch/tasks.py (1)
110-228: Add comprehensive Google-style docstring.The function is missing a complete docstring documenting the new
send_eventsparameter. Per coding guidelines, all Python functions should have Google-style docstrings.As per coding guidelines.
Example docstring:
@flow(name="branch-rebase", flow_run_name="Rebase branch {branch}") async def rebase_branch(branch: str, context: InfrahubContext, send_events: bool = True) -> None: """Rebase a branch onto the default branch and apply post-rebase migrations. This flow performs conflict detection, schema validation, rebases the branch, applies schema migrations, triggers IPAM reconciliation, runs branch migrations, refreshes diffs, and optionally emits rebase and node change events. Args: branch: Name of the branch to rebase. context: Infrahub context for the operation. send_events: Whether to emit events after successful rebase. Raises: ValidationError: If conflicts exist or validation constraints fail. BranchNotFoundError: If the specified branch does not exist. MigrationFailureError: If migrations fail during the rebase process. """backend/infrahub/cli/db.py (2)
283-310: Enhance docstring to Google-style format.The docstring is present but minimal. Per coding guidelines, Python functions should have Google-style docstrings with Args and Returns sections.
As per coding guidelines.
Example enhancement:
async def detect_migration_to_run( current_graph_version: int, migration_number: int | str | None = None ) -> Sequence[MigrationTypes]: """Return a sequence of migrations to apply to upgrade the database. This function checks the current database version and determines which migrations need to be applied. It can either check all pending migrations or validate a specific migration number. Args: current_graph_version: The current graph version of the database. migration_number: If provided, check only this specific migration number. Returns: A sequence of migration instances to apply, or an empty sequence if no migrations are needed or if the requested migration is already applied. """
356-383: Consider adding docstring Args section and verify session context.Two observations:
Docstring enhancement: Per coding guidelines, the docstring should include an Args section documenting the
dbparameter.Session context: A past review comment suggested that database operations should be wrapped in
async with db.start_session()context. Theload_schemacall at line 373 and the subsequentrebase_branchcall may benefit from explicit session management for each branch iteration to ensure proper transaction handling.As per coding guidelines (for docstring) and based on learnings (for session context).
Example refactor:
async def trigger_rebase_branches(db: InfrahubDatabase) -> None: - """Trigger rebase of non-default branches, also triggering migrations in the process.""" + """Trigger rebase of non-default branches, also triggering migrations in the process. + + Args: + db: The database object. + """ branches = [b for b in await Branch.get_list(db=db) if b.name not in [registry.default_branch, GLOBAL_BRANCH_NAME]] if not branches: return rprint(f"Planning rebase and migrations for {len(branches)} branches: {', '.join([b.name for b in branches])}") for branch in branches: if branch.graph_version == GRAPH_VERSION: rprint( f"Ignoring branch rebase and migrations for '{branch.name}' (ID: {branch.uuid}), it is already up-to-date" ) continue rprint(f"Rebasing branch '{branch.name}' (ID: {branch.uuid})...", end="") try: - await registry.schema.load_schema(db=db, branch=branch) - await rebase_branch( - branch=branch.name, - context=InfrahubContext.init( - branch=branch, account=AccountSession(auth_type=AuthType.NONE, authenticated=False, account_id="") - ), - send_events=False, - ) + async with db.start_session() as db_session: + await registry.schema.load_schema(db=db_session, branch=branch) + await rebase_branch( + branch=branch.name, + context=InfrahubContext.init( + branch=branch, account=AccountSession(auth_type=AuthType.NONE, authenticated=False, account_id="") + ), + send_events=False, + ) rprint(SUCCESS_BADGE) except (ValidationError, MigrationFailureError): rprint(FAILED_BADGE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/infrahub/cli/db.py(7 hunks)backend/infrahub/cli/upgrade.py(6 hunks)backend/infrahub/core/branch/enums.py(1 hunks)backend/infrahub/core/branch/models.py(3 hunks)backend/infrahub/core/branch/tasks.py(5 hunks)backend/infrahub/core/constants/__init__.py(1 hunks)backend/infrahub/core/migrations/exceptions.py(1 hunks)backend/infrahub/core/migrations/graph/__init__.py(2 hunks)backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py(2 hunks)backend/infrahub/core/migrations/runner.py(1 hunks)backend/infrahub/core/migrations/shared.py(2 hunks)backend/infrahub/events/branch_action.py(2 hunks)backend/infrahub/graphql/types/branch.py(2 hunks)backend/infrahub/task_manager/event.py(2 hunks)backend/infrahub/task_manager/models.py(1 hunks)backend/infrahub/workflows/catalogue.py(2 hunks)backend/tests/unit/core/migrations/test_runner.py(1 hunks)backend/tests/unit/workflows/test_models.py(1 hunks)changelog/+upgrade-process.changed.md(1 hunks)docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx(1 hunks)docs/docs/reference/infrahub-events.mdx(2 hunks)schema/schema.graphql(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/infrahub/events/branch_action.py
- backend/infrahub/core/migrations/exceptions.py
- backend/infrahub/graphql/types/branch.py
- backend/infrahub/core/migrations/shared.py
- backend/infrahub/core/branch/enums.py
- docs/docs/reference/infrahub-cli/infrahub-upgrade.mdx
- backend/infrahub/task_manager/models.py
- backend/infrahub/core/migrations/runner.py
🧰 Additional context used
📓 Path-based instructions (9)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/constants/__init__.pybackend/tests/unit/workflows/test_models.pybackend/infrahub/task_manager/event.pybackend/infrahub/cli/db.pybackend/infrahub/core/branch/models.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/branch/tasks.pybackend/infrahub/cli/upgrade.pybackend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.pybackend/tests/unit/core/migrations/test_runner.pybackend/infrahub/core/migrations/graph/__init__.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/infrahub/core/constants/__init__.pybackend/tests/unit/workflows/test_models.pybackend/infrahub/task_manager/event.pybackend/infrahub/cli/db.pybackend/infrahub/core/branch/models.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/branch/tasks.pybackend/infrahub/cli/upgrade.pybackend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.pybackend/tests/unit/core/migrations/test_runner.pybackend/infrahub/core/migrations/graph/__init__.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/workflows/test_models.pybackend/tests/unit/core/migrations/test_runner.py
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
changelog/+upgrade-process.changed.mddocs/docs/reference/infrahub-events.mdx
docs/docs/reference/**/*.{md,mdx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write reference documentation in
docs/docs/reference/
Files:
docs/docs/reference/infrahub-events.mdx
docs/**/*.mdx
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Docusaurus markdown/MDX features in documentation
docs/**/*.mdx: How-to guides should begin title with "How to..." and follow the Diataxis How-to structure (intro, prerequisites, step-by-step, validation, advanced, related)
Use imperative, task-focused instructions in guides; avoid digressions
Topics should use titles like "About..." or "Understanding..." and follow the Diataxis Topics structure (concepts, background, architecture, mental models, connections, alternatives, further reading)
Define new terms on first use; prefer domain-relevant terminology; keep naming consistent with Infrahub UI/data model
Files:
docs/docs/reference/infrahub-events.mdx
docs/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Develop documentation in
docs/, preview withinvoke docs.build docs.serve, and validate withinvoke docs.validate
Files:
docs/docs/reference/infrahub-events.mdx
**/*.mdx
📄 CodeRabbit inference engine (.github/instructions/documentation.instructions.md)
**/*.mdx: Structure documentation primarily as How-to Guides and Topics (explanations) following the Diataxis framework
Use a professional, approachable tone; avoid jargon unless defined; use plain language with technical precision
Write concise, direct, active sentences
Be informative over promotional; focus on explaining how and why
Maintain consistent, predictable structure across sections and documents
For Guides: use conditional imperatives (e.g., "If you want X, do Y")
For Guides: focus on practical tasks and problems, not tools
For Guides: address the user directly with imperative verbs (e.g., "Configure...", "Create...")
For Guides: keep focus on the specific goal; avoid digressions into explanations
For Guides: title in YAML frontmatter should clearly state the problem and begin with "How to..."
For Guides: include an Introduction stating the problem/goal, context, and what the user will achieve
For Guides: include a Prerequisites/Assumptions section listing requirements and prior knowledge
For Guides: provide step-by-step instructions with numbered steps; include code snippets/screenshots/tabs/callouts as needed
For Guides: include a Validation/Verification section with checks, example outputs, and potential failure points
For Guides: include a Related Resources section linking to relevant guides/topics/reference
For Topics: title in YAML frontmatter should clearly indicate the topic; consider starting with "About..." or "Understanding..."
For Topics: include an Introduction with overview, why it matters, and questions answered
For Topics: include sections for Concepts & Definitions
For Topics: include Background & Context (history, design rationale, constraints)
For Topics: include Architecture & Design details/diagrams when applicable
For Topics: include Mental Models (analogies/comparisons)
For Topics: connect to other concepts and integration points
For Topics: include Alternative Approaches with pros/cons where relevant
For Topics: include a Further...
Files:
docs/docs/reference/infrahub-events.mdx
docs/!(node_modules)/**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
docs/!(node_modules)/**/*.{md,mdx}: Lint documentation prose with Vale
Ensure Vale style checks pass for documentation
Files:
docs/docs/reference/infrahub-events.mdx
🧠 Learnings (4)
📓 Common learnings
Learnt from: ogenstad
Repo: opsmill/infrahub PR: 7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.663Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
📚 Learning: 2025-10-30T12:43:45.663Z
Learnt from: ogenstad
Repo: opsmill/infrahub PR: 7447
File: schema/schema.graphql:324-329
Timestamp: 2025-10-30T12:43:45.663Z
Learning: In the GraphQL schema (schema/schema.graphql), the BranchStatus enum includes NEED_UPGRADE_REBASE. Branches with NEED_UPGRADE_REBASE status must support conflict resolution capabilities.
Applied to files:
changelog/+upgrade-process.changed.mdschema/schema.graphqlbackend/infrahub/core/branch/models.pybackend/infrahub/workflows/catalogue.pybackend/infrahub/core/branch/tasks.py
📚 Learning: 2025-08-21T11:04:26.357Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/instructions/documentation.instructions.md:0-0
Timestamp: 2025-08-21T11:04:26.357Z
Learning: Applies to **/*.mdx : Ensure content is accurate and reflects the latest Infrahub version
Applied to files:
docs/docs/reference/infrahub-events.mdx
📚 Learning: 2025-07-22T08:13:56.198Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-22T08:13:56.198Z
Learning: Documentation generated for Infrahub must reflect its novel approach, providing clarity around new concepts and demonstrating integration with familiar patterns from existing tools like Git, infrastructure-as-code, and CI/CD pipelines
Applied to files:
docs/docs/reference/infrahub-events.mdx
🧬 Code graph analysis (8)
backend/tests/unit/workflows/test_models.py (1)
backend/infrahub/workflows/models.py (1)
WorkflowParameter(38-41)
backend/infrahub/cli/db.py (7)
backend/infrahub/auth.py (2)
AccountSession(44-52)AuthType(38-41)backend/infrahub/context.py (1)
InfrahubContext(33-53)backend/infrahub/core/branch/tasks.py (1)
rebase_branch(111-253)backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/exceptions.py (1)
ValidationError(319-341)backend/infrahub/core/migrations/graph/__init__.py (2)
get_migration_by_number(111-126)get_graph_migrations(100-108)
backend/infrahub/core/branch/models.py (1)
backend/infrahub/core/node/standard.py (1)
create(102-116)
backend/infrahub/workflows/catalogue.py (1)
backend/infrahub/workflows/models.py (1)
WorkflowDefinition(44-121)
backend/infrahub/core/branch/tasks.py (6)
backend/infrahub/core/migrations/exceptions.py (1)
MigrationFailureError(1-4)backend/infrahub/core/migrations/runner.py (3)
MigrationRunner(17-54)has_migrations(35-36)run(38-54)backend/infrahub/events/branch_action.py (1)
BranchMigratedEvent(133-156)backend/infrahub/workflows/utils.py (1)
add_tags(22-48)backend/infrahub/workers/dependencies.py (1)
get_workflow(122-123)backend/infrahub/core/branch/models.py (1)
get_by_name(135-152)
backend/infrahub/cli/upgrade.py (2)
backend/infrahub/core/initialization.py (3)
initialization(140-215)get_root_node(49-63)initialize_registry(92-127)backend/infrahub/cli/db.py (3)
detect_migration_to_run(283-310)migrate_database(313-353)trigger_rebase_branches(356-383)
backend/infrahub/core/migrations/graph/m043_backfill_hfid_display_label_in_db.py (1)
backend/infrahub/core/migrations/shared.py (1)
MigrationRequiringRebase(229-247)
backend/tests/unit/core/migrations/test_runner.py (1)
backend/infrahub/core/migrations/runner.py (2)
MigrationRunner(17-54)has_migrations(35-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (15)
schema/schema.graphql (2)
147-147: ✅ graph_version field properly implemented.The
graph_version: Intfield is correctly defined in the schema and fully backed by the backend implementation. The GraphQL type exposes it as an optional field, the branch model includes it asint | None, and it's properly initialized toGRAPH_VERSIONduring branch creation. The field is also appropriately used in branch migration workflows.
327-327: ✅ NEED_UPGRADE_REBASE enum value correctly added and properly integrated.Verification confirms the design is correct:
- Enum properly defined in backend (
backend/infrahub/core/branch/enums.py:7)- Status assignment logic in
backend/infrahub/core/branch/tasks.pycorrectly sets during upgrade process and clears after migration- Validation logic intentionally excludes NEED_UPGRADE_REBASE from the
check_need_rebase_status()check, allowing conflict resolution mutations while NEED_REBASE branches remain restricted- This aligns with the stated design: branches with NEED_UPGRADE_REBASE intentionally support conflict resolution and should not have the same mutation restrictions as NEED_REBASE branches
backend/infrahub/workflows/catalogue.py (2)
233-239: LGTM!The BRANCH_MIGRATE workflow definition is correctly structured and follows the established pattern for branch workflows. The DATABASE_CHANGE tag is appropriate since migrations modify the database.
651-651: LGTM!The BRANCH_MIGRATE workflow is correctly added to the WORKFLOWS list with proper placement.
backend/infrahub/cli/upgrade.py (4)
14-19: LGTM!The import additions are well-organized and all imported functions are properly utilized in the upgrade flow.
Also applies to: 34-40
54-54: LGTM!The
rebase_branchesparameter is well-named with a safe default and clear help text.
72-88: LGTM!The migration detection flow is correct, and the database driver is properly closed on the early return path. This addresses the previously raised concern.
113-118: LGTM!The branch rebase logic is correctly conditionalized and well-documented with the comment. The placement after migrations and schema updates is appropriate.
backend/infrahub/core/migrations/graph/__init__.py (2)
50-53: LGTM!The refactoring to use the
MigrationTypestype alias improves code maintainability and consistency across the migration framework.
111-126: LGTM!The implementation properly handles numeric parsing with error handling and provides clear error messages. The logic for finding migrations by class name is straightforward and correct.
backend/infrahub/core/branch/tasks.py (2)
89-94: LGTM!The migration execution now correctly re-raises
MigrationFailureErrorafter logging, addressing the past review concern about swallowing exceptions. This ensures failures propagate properly to the caller.
96-107: LGTM!The branch status reset and event emission logic is correct. The status is properly cleared from
NEED_UPGRADE_REBASEtoOPENafter successful migrations, and events are conditionally emitted based on thesend_eventsparameter.backend/infrahub/cli/db.py (3)
21-26: LGTM!The import additions are well-organized and all imported types and functions are properly utilized throughout the file.
Also applies to: 38-40, 66-66
112-120: LGTM!The migration flow is correctly restructured to detect migrations before applying them. The early return logic properly handles both check mode and empty migration lists.
313-353: LGTM!The
migrate_databasefunction is correctly updated with proper docstring and signature. The migration execution loop properly handles validation and error reporting.
| async def create(self, db: InfrahubDatabase) -> bool: | ||
| self.graph_version = GRAPH_VERSION | ||
| return await super().create(db=db) |
There was a problem hiding this comment.
Set graph_version during branch rebases
create() now seeds graph_version, but rebase() still leaves whatever stale value the branch carried before the migration. After a successful rebase the branch should advertise the current GRAPH_VERSION; otherwise the new upgrade flow keeps treating it as out-of-date and may keep it in NEED_UPGRADE_REBASE. Please assign and persist the fresh version inside rebase() once the transaction succeeds. For example:
self.branched_from = at.to_string()
self.status = BranchStatus.OPEN
+ self.graph_version = GRAPH_VERSION
await self.save(db=db)Also applies to: 476-493
🤖 Prompt for AI Agents
In backend/infrahub/core/branch/models.py around lines 264-266 (and similarly
476-493), rebase() currently leaves the branch's stale graph_version untouched;
after a successful rebase you must assign self.graph_version = GRAPH_VERSION and
persist that change in the database inside the successful transaction/commit
path. Update rebase() to set the new GRAPH_VERSION on the instance and call the
model's save/update method (or run an UPDATE via the db session) only after the
rebase transaction succeeds so the refreshed graph_version is stored and the
branch no longer appears as needing an upgrade rebase.
These changes add a new flow to apply migrations (schema/data ones) inside branches. The upgrade process (via the provided command) is also updated with a new
--rebase-branchesflag to trigger branch rebases as part of the upgrade process.A new
MigrationRequiringRebaseclass is introduced with a couple of methods that need to be defined on classes inheriting it. One method defines what to do on the default branch while the other one defines what to do for all other branches.The branch nodes can have a new
NEED_UPGRADE_REBASEstatus and they also now have agraph_versionproperty to know if a branch is up-to-date regarding the graph structure.A new prefect event is also introduced and triggered when the
branch-migrateflow completes for a branch.Summary by CodeRabbit
New Features
Schema Updates
Documentation
Tests